Add stick overlays to OSD #7167

pkruger · 2018-12-03T09:32:38Z

No description provided.

mikeller · 2018-12-03T09:47:00Z

src/main/interface/settings.c

+#ifdef USE_OSD_STICK_OVERLAY
+    { "osd_stick_overlay_left",      VAR_UINT16  | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_LEFT]) },
+    { "osd_stick_overlay_right",     VAR_UINT16  | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_RIGHT]) },
+    { "osd_horizontal_char",         VAR_UINT8   | MASTER_VALUE, .config.minmax = { 0, 255 }, PG_OSD_CONFIG, offsetof(osdConfig_t, overlay_horizontal_char) },


If we go for this solution, we should go for fixed characters. If users like different styles, they can use different fonts, or they can edit their font.

@mikeller
Tricky one. The default characters -, | and + used for the crosses have gaps between them, which looks a little blocky. (I have added a videos to the issue #3988). Users can edit them specifically, but then it messes up other places where they are used, like the startup screen of Betaflight. (to enter the OSD menu). Having them configurable enables users to either use the defaults as is or to change characters in their fonts that they never use, like the compass characters or GPS satellite characters for example.

It may be better to use single array variable. Using different font is quite complicated, so user should be experienced enough to use it.

I still think we should not try to address this by introducing parameters for the characters to be used. How about this: We hardcode on '-', '|', and '+' for now, and start working on adding better glyphs to the fonts (there are unused characters available). Once we have the updated fonts published in configurator, we'll change the defines in the firmware for the glyphs to be used to use the new custom made ones?

@mikeller Makes sense. If we make them configurable, most people might never change them anyway. Ok. I'll remove the configurable characters and hard code them to '-', '|', and '+'.

mikeller · 2018-12-03T09:52:11Z

src/main/io/osd.c

@@ -31,6 +31,7 @@
 #include <string.h>
 #include <ctype.h>
 #include <math.h>
+#include <drivers/max7456.h>


This should go into the section with the other drivers/ headers.

Removed, no longer required.

mikeller · 2018-12-03T09:52:45Z

src/main/io/osd.c

+static void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos)
+{
+
+    for (uint16_t x=0; x<OSD_STICK_OVERLAY_WIDTH; x++) {


Please use space around operators, except on the inside of brackets.

mikeller · 2018-12-03T09:56:33Z

src/main/io/osd.c

@@ -1198,6 +1265,15 @@ static void osdDrawElements(void)
        osdDrawSingleElement(OSD_LOG_STATUS);
    }
 #endif
+
+#ifdef USE_OSD_STICK_OVERLAY
+    if ((VISIBLE(osdConfig()->item_pos[OSD_STICK_OVERLAY_LEFT])) &&


Hmm, this is a bit odd, and unintuitive - if we define separate item_pos items for the individual stick overlays, then we should also enable them individually.

I guess that makes sense. I assumed they will always be used together, but there may be exceptions. Fixed.

It's not so much that I expect users to use the OSD with only one enabled. But the way all other OSD elements work is 'user enables element => element shows'. Deviating from this behaviour here for no actual benefit will just be confusing for users if they (for whatever reason) enable an element and it does not show up.

mikeller · 2018-12-03T09:58:43Z

src/main/io/osd.c

@@ -1236,6 +1312,14 @@ void pgResetFn_osdConfig(osdConfig_t *osdConfig)
    osdConfig->timers[OSD_TIMER_1] = OSD_TIMER(OSD_TIMER_SRC_ON, OSD_TIMER_PREC_SECOND, 10);
    osdConfig->timers[OSD_TIMER_2] = OSD_TIMER(OSD_TIMER_SRC_TOTAL_ARMED, OSD_TIMER_PREC_SECOND, 10);

+    osdConfig->item_pos[OSD_STICK_OVERLAY_LEFT] = OSD_POS(1,(max7456GetRowsCount()-OSD_STICK_OVERLAY_HEIGHT));


OSD isn't limited to MAX7456 devices, so max7456GetRowsCount() is wrong here.

I have removed this.

mikeller · 2018-12-03T10:01:55Z

src/main/io/osd.h

@@ -101,6 +105,8 @@ typedef enum {
    OSD_FLIP_ARROW,
    OSD_LINK_QUALITY,
    OSD_TOTAL_DIST,
+    OSD_STICK_OVERLAY_LEFT,
+    OSD_STICK_OVERLAY_RIGHT,


We need to increment the version of the osdConfig parameter group here, as we are increasing OSD_ITEM_COUNT. See https://github.com/betaflight/betaflight/blob/master/docs/development/ParameterGroups.md

Oops. Thanks, now fixed.

pkruger · 2018-12-03T11:39:40Z

Updated with all the changes from the review above - thanks.

ledvinap · 2018-12-03T12:20:23Z

src/main/interface/settings.c

+#ifdef USE_OSD_STICK_OVERLAY
+    { "osd_stick_overlay_left",      VAR_UINT16  | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_LEFT]) },
+    { "osd_stick_overlay_right",     VAR_UINT16  | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_RIGHT]) },
+    { "osd_horizontal_char",         VAR_UINT8   | MASTER_VALUE, .config.minmax = { 0, 255 }, PG_OSD_CONFIG, offsetof(osdConfig_t, overlay_horizontal_char) },


It may be better to use single array variable. Using different font is quite complicated, so user should be experienced enough to use it.

ledvinap · 2018-12-03T12:23:21Z

src/main/io/osd.c

@@ -1146,6 +1146,53 @@ static bool osdDrawSingleElement(uint8_t item)
    return true;
 }

+#ifdef USE_OSD_STICK_OVERLAY
+static void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos)


Oops. Fixed. Thanks.

Not so sure about the array variable, then the user would have to know which index is which parameter. With distinct names it seems easier.

You can define enum for characters ... osdConfig()->overlay[osdStickHorizontal]

Also, the characters will no longer be configurable as discussed above, so fewer parameters now.

ledvinap · 2018-12-03T12:23:35Z

src/main/io/osd.c

+static void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos)
+{
+
+    for (uint8_t x = 0; x < OSD_STICK_OVERLAY_WIDTH; x++) {


unsigned x = 0

What do you mean here?

Using non-native types on ARM si suboptimal (gcc emits uint8_t -> unsigned on almost every access).
(my opinion, not generally shared between BF programmers): You want unsigned variable, but size is not important. Using uint8_t looks like you want to limit variable size for some reason. Using uint32_t (which is equivalent to unsigned on target processor) looks like there is some reason to use exactly 32 bits. It is confusing when reviewing code (you have to think about it a little bit).
I prefer to stick to signed (sign importance is stressed), int - just a number and unsigned - unsigned number is enough (usually to avoid signed/unsigned warning with sizeof etc.)

Interesting, that's correct. Just unsigned by itself is not very portable, but I guess the days of 16-bit processors are over. I'll change it to unsigned x. The same goes for void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos), to void osdDrawStockOverlayAxis(unsigned xpos, unsigned ypos) then? Not quite...

src/main/io/osd.c

ledvinap · 2018-12-03T12:36:52Z

src/main/io/osd.c

+#ifdef USE_OSD_STICK_OVERLAY
+    if (VISIBLE(osdConfig()->item_pos[OSD_STICK_OVERLAY_LEFT])) {
+        osdDrawStickOverlayAxisItem(OSD_STICK_OVERLAY_LEFT);
+        osdDrawStickOverlayCursor(OSD_STICK_OVERLAY_LEFT, THROTTLE, YAW);


(this works only in mode 2)

Now updated to support both mode 1,2,3 and 4.

mikeller · 2018-12-05T10:43:53Z

Fixes #3988.

pkruger · 2018-12-06T09:32:14Z

Changes added as requested above as well as support for radio modes 1, 2, 3 and 4.

ledvinap · 2018-12-06T11:32:06Z

src/main/io/osd.c

@@ -103,6 +103,11 @@
 #define VIDEO_BUFFER_CHARS_PAL    480
 #define FULL_CIRCLE 360

+#define STICK_OVERLAY_HORIZONTAL_CHAR   0x2D


It may be cleaner to use character constants (+,-,|,0)

ledvinap · 2018-12-06T11:33:13Z

src/main/io/osd.c

@@ -143,7 +148,27 @@ typedef struct statistic_s {
    uint8_t min_link_quality;
 } statistic_t;

+typedef struct radioControls_s {
+    rc_alias_e left_vertical;


(uint8_t may be used to save some space, enum in 32bit)

ledvinap · 2018-12-06T11:41:58Z

src/main/io/osd.c

+    }
+
+    uint8_t x_pos =  (uint8_t)scaleRange(constrain(rcData[horizontal_channel], PWM_RANGE_MIN, PWM_RANGE_MAX), PWM_RANGE_MIN, PWM_RANGE_MAX, 0, OSD_STICK_OVERLAY_WIDTH);
+    uint8_t y_pos =  (uint8_t)scaleRange(PWM_RANGE_MAX - constrain(rcData[vertical_channel], PWM_RANGE_MIN, PWM_RANGE_MAX), PWM_RANGE_MIN, PWM_RANGE_MAX, 0, OSD_STICK_OVERLAY_HEIGHT) + OSD_STICK_OVERLAY_HEIGHT - 1;


scaleRange can invert direction, just map to OSD_STICK_OVERLAY_HEIGHT, 0

Also this equation is IMO incorrect, it works only when PWM_RANGE_MAX = 2*PWM_RANGE_MIN

Scalerange: uint8_t y_pos = (uint8_t)scaleRange(constrain(rcData[vertical_channel], PWM_RANGE_MIN, PWM_RANGE_MAX), PWM_RANGE_MIN, PWM_RANGE_MAX, OSD_STICK_OVERLAY_HEIGHT, 0) + OSD_STICK_OVERLAY_HEIGHT - 1;
Doesn't work, not sure why.

Why do you think it only works with PWM_RANGE_MAX = 2*PWM_RANGE_MIN?

ledvinap · 2018-12-06T11:46:33Z

src/main/io/osd.c

@@ -1236,6 +1313,26 @@ static void osdDrawElements(void)
        osdDrawSingleElement(OSD_LOG_STATUS);
    }
 #endif
+
+#ifdef USE_OSD_STICK_OVERLAY
+#ifdef USE_OSD_PROFILES


This code construction introduces double opening {, confusing some editors (yes, some of us are using emacs ;-)

IMO USE_OSD_PROFILES should override VISIBLE macro(or use some other abstraction), so that OSD profiles are transparent to this type of code

Oh yes, I like that idea. Done.

mikeller · 2018-12-08T00:47:12Z

src/main/interface/settings.c

@@ -1088,6 +1088,12 @@ const clivalue_t valueTable[] = {
    { "osd_log_status_pos",         VAR_UINT16  | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_LOG_STATUS]) },
 #endif

+#ifdef USE_OSD_STICK_OVERLAY
+    { "osd_stick_overlay_left",        VAR_UINT16  | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_LEFT]) },


These should be following the naming convention for element positions: <element>_pos.

mikeller · 2018-12-08T00:53:32Z

docs/OSD Stick Overlay.md

+
+## Configuration
+
+Currently (Configurator 10.4.0) OSD Stick Overlays can only be configured via the CLI.  Layout your OSD using the configurator and save.  


Instead of having this comment here, we should add support for positioning these elements to configurator - the data for it is already sent over MSP, since the entire item_pos array is sent. If we default the TX mode to 2, then this should cover it for most users.

Ok, I'll remove all references to Configurator 10.4.0 and assume that the Configurator supports it.

mikeller · 2018-12-08T00:54:51Z

src/main/io/osd.c

+    for (unsigned x = 0; x < OSD_STICK_OVERLAY_WIDTH; x++) {
+        for (unsigned  y = 0; y < OSD_STICK_OVERLAY_HEIGHT; y++) {
+            // draw the axes, vertical and horizonal
+            if ((x == ((OSD_STICK_OVERLAY_WIDTH-1)/2)) && (y == (OSD_STICK_OVERLAY_HEIGHT-1)/2)) {


Spaces around operators are still missing in a lot of places.

pkruger · 2018-12-08T12:58:07Z

Updated with review changes from above.

mikeller · 2018-12-08T20:58:33Z

src/main/io/osd.c

 static statistic_t stats;
+static const radioControls_t radioModes[4] = {


This needs to be made conditional, or the build will fail with a warning for targets without USE_OSD_PROFILES. (Protip: Having a look at the result of the CI checks will reveal this type of problem.)

Thanks, yes I should probably use the CI checks and reap the benefits. Now passing...

mikeller · 2018-12-09T11:46:26Z

src/main/io/osd.h

@@ -233,3 +243,4 @@ void osdSuppressStats(bool flag);
 void setOsdProfile(uint8_t value);
 uint8_t getCurrentOsdProfileIndex(void);
 void changeOsdProfileIndex(uint8_t profileIndex);
+bool osdElementVisible(uint16_t value);


I don't think this is used anywhere externally (or meant to be), so this shouldn't be here.

It's used in cms.c, i.e. in osd.h VISIBLE(x) is defined as: #define VISIBLE(x) osdElementVisible(x).

pkruger · 2018-12-10T12:12:16Z

Also added the stick overlays to the OSD Active Elements menu.

mikeller · 2018-12-15T04:42:29Z

@pkruger: Can I ask you to also open a pull request in configurator to add the stick overlays to the OSD tab?

mikeller · 2018-12-16T01:00:05Z

@pkruger: I finally got around to test flying the stick overlays, great work! One thing that I noticed is that with my setup that is cutting off some of the outermost rows of characters on the screen, the stick overlays are covering a large area, in particular they are very high, appearing about twice as high as wide.
I don't know if this is the case on all displays, but I suspect so. How about reducing their height to 5 rows to make them appear more square, at least as an option?

To counteract the resulting loss in vertical resolution, we could change it so that every character (except for the center row) is used for two vertical positions, one with the indicator in the lower half, another one with it in the upper half. This way, we'd practically double the vertical resolution at the expense of 2 extra characters, which sounds like a good deal to me.

Thoughts?

mikeller requested changes Dec 3, 2018

View reviewed changes

mikeller added this to the 4.0 milestone Dec 3, 2018

mikeller added the RN: MINOR FEATURE label Dec 3, 2018

pkruger force-pushed the 3988-feature-add-sticks-verlay-to-in-flight-OSD branch 2 times, most recently from ede35f3 to 7b00452 Compare December 3, 2018 11:39

pkruger mentioned this pull request Dec 3, 2018

Add sticks overlay to in flight OSD #3988

Closed

ledvinap reviewed Dec 3, 2018

View reviewed changes

pkruger force-pushed the 3988-feature-add-sticks-verlay-to-in-flight-OSD branch from 7b00452 to 111391f Compare December 6, 2018 09:31

ledvinap reviewed Dec 6, 2018

View reviewed changes

mikeller requested changes Dec 8, 2018

View reviewed changes

pkruger force-pushed the 3988-feature-add-sticks-verlay-to-in-flight-OSD branch 2 times, most recently from 52451f8 to 541d5d7 Compare December 8, 2018 12:57

mikeller requested changes Dec 8, 2018

View reviewed changes

pkruger force-pushed the 3988-feature-add-sticks-verlay-to-in-flight-OSD branch from 541d5d7 to ef459cb Compare December 9, 2018 10:44

Add OSD Stick Overlay feature - fix issue 3988

ef459cb

mikeller requested changes Dec 9, 2018

View reviewed changes

mikeller approved these changes Dec 10, 2018

View reviewed changes

mikeller merged commit e1e942b into betaflight:master Dec 10, 2018


		## Configuration

		Currently (Configurator 10.4.0) OSD Stick Overlays can only be configured via the CLI. Layout your OSD using the configurator and save.

		static statistic_t stats;
		static const radioControls_t radioModes[4] = {

Add stick overlays to OSD #7167

Add stick overlays to OSD #7167

Conversation

pkruger commented Dec 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkruger commented Dec 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkruger Dec 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkruger Dec 4, 2018 • edited

Choose a reason for hiding this comment

mikeller commented Dec 5, 2018

pkruger commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkruger commented Dec 8, 2018

Choose a reason for hiding this comment

pkruger Dec 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkruger commented Dec 10, 2018

mikeller commented Dec 15, 2018

mikeller commented Dec 16, 2018

pkruger commented Dec 3, 2018 •

edited

pkruger Dec 5, 2018 •

edited

pkruger Dec 4, 2018 •

edited

pkruger Dec 9, 2018 •

edited